git-commit-hook - lets start where commits begin.
in theory this is git's post-recieve, to use it you need to add this line to post-receive in the hooks directory
/usr/bin/php /var/www/mtrack/bin/git-commit-hook.php post > /tmp/githooklog
Note, I renamed the file with a php extension (as it makes editing the file nicer - auto detect file extension etc.. for syntax highlighting)
Things that where not quite right about this file.
- I think in theory, the class and the 'runner' component should be split out, this way the end user can modify the runner component to suit. (eg. bin/git-post-recieve-hook.example.php, and inc/commit-hook-git.php)
- The whoami() call in the script assumes that you use git via ssh, I normally use git via http, and in the case I was using it for we use subversion as the front end (as the developers could just not 'get' git). and we have a post-commit hook in subversion that commits into the git repository. In our case we had to hard code the whoami to be www-data, and set up a user for that, otherwise the script would return permission denied..
- Better logging of hook scripts. - finding problems in the hook script took quite a while, it was only after I started adding some simple log output code did I find out why nothing was running.
- Slight quible with the design, PEAR has a nice feature System::which(), to find out where the real binaries are, mtrack uses a list in the config file. I'd rather not have to configure something like that, unless all else fails..
svn-commit-hook
Again pretty much the same applies here, breaking the file into class+script part would make it more flexible, in our case, since we want to use it for pre-commit hook, and our real repository is actually git, we have to kludge the pre-commit script to think that the commit is being done to the other repository, so it can find the relivant issue numbers. (I'm still testing this part..)
I also noticed that when testing it might have tried to reject a commit which was a PHP file with a syntax error, normally not a bad idea, but the action in this case was that the file was being deleted... - need to look further into that.
Commit hooks in general.
One of the features I was pretty keen to use was the tracking of commit's against tickets. We request the developers to do a task, when they commit, they have to tell us which issue they are addressing. we review the commit, and approve it.
This is where we got caught by the current mtrack code. It appears Wez is using it to track time taken as well as issue numbers. and there is a huge regular expression to deal with parsing the bug number and the time taken. It also automatically closes an issue if you prefix the bug number with 'fixed' or similar.
For our usage, we did not want the developers to be able to close an issue, they will need to go into the mtrack web interface, modify the issue, and say it's completed.In which case we can then go in, review the commits, and either say it's closed, or re-open it and request more tidy ups.
To make this change happen I had to modify the MTrackCommitChecker constructor and make timepat blank, along with forcing the actions to just be 'ref' an all cases. I suspect the best way to handle this kind of customization is to have a 'local' directory and put all the 'overrides classes in there', and use include path, base classes to enable the overriding of features like this.
Look and feel.
Moving on to the look and feel, for our purposes, we did not really need the wiki at present The two key reports that where needed where 'current todo' and 'pending approval'. Having to drill down on the report page to find these is a bit klunky. The only current way to modify the navigation bar is to change the code in the inc/web.php which deals with headers (This shout's use templates....)
Javascript, CSS, HTML and PHP.
My sense is that mtrack was developed to solve a problem, It was never designed as the perfect example of good coding practice. In any file on the system you could find any or all of these 'HTML, CSS Javascript and PHP'.
For new projects I work on, it is practically forbidden to mix these in any other way than these few cases:
- Javascript variables can be injected into pages using <flexy:toJavascript> in the templates
- PHP should only output raw HTML via the template engine, and ocassionally if there is some special formating required. I don't think I've switched between PHP and HTML output in a single file for years.
- CSS files should be kept seperate.
- Javascript files should be self contained, and seperate from the pages
There is some seperation of Javascript and CSS from the application, I even started moving some of the Javascript into a seperate file. However in doing so I came across one of my least favourite design aspects of mtrack, the css.php and js.php files..
He's the logic, pages load faster if you have less assets (eg. 1 css file and 1 js file, and if you are really clever, 1 image...) and they load even faster if you remove white space and run things like my javascript packer.
There is one rather large drawback to all this though, even just merging files like this make debugging significantly more complex. (as the size of files increases, and you loose the relationship between 'source of the problem and location that it is shown as). This is particularly true for the CSS at present, If you need to modify the CSS, finding the right line and file using firebug, is relatively pointless.. (although not impossible)
As I tried to move some of the JS code around I quickly found that debugging in this manner is quite painfull.
The correct solution to this is what my Pman framework does (mostly).
a) have a debug mode (which is on by default)
In the debug mode, each page outputs all the included Javascript and CSS files from their original sources. For the most part this is actually perfectly usable, as after the first load, most files are cached anyway.
b) have a optimize mode (for those special occasions)
In the optimize mode, that rather than outputting a <script> or <link> tag for each CSS/javascript file, you either compile (with cache checking) the javascript or css files to include, and just include one <script> and one <link>.
Adding features - Changed files and expandable Diffs in the comments.
One of the features I needed based on the workflow discussed was to be able to preview the changes by looking at the specific issue and it's changes. eg. "issue #10 tidy up code in the listing table". What I would need to see, is a list of files that have modified, and by clicking on them, expand out and see the diff.
To make that work, I ended up copying the changeset.php file into jschangeset.php, and removing most of the display code. - In hindsight, it might just have worked by making a small change to changeset.php. This responded to a request for ?part=1 with just the HTMLized version of the diff for the change requested.
Then all that was needed was to change the output code for the 'comments' on the issue log, so that when it found
[changeset:repo/project,change]
in the comment, it used the get_change_data code from log.php to fetch a list of changed files, and output them with a onclick handler on the filename, which in turn did a $().load() call to display the diff just below the list of files.
Adding features - Where are we in the log.
In our situation, the developers commit to the subversion system, which in turn commits to git, we make a copy with all the current commits available on the development domain so we can review the results. Once all the changes have been complete, we would normally just ssh in, and manually pull the changes into the live site.
This has a slight bottleneck, in terms of only the god^C^Cadmin around here knows how to type 'git pull'.
So ideally we should be able to look at the changelog, see where the live site is in relationship to all the recent changes. From there we should be able to see how many of the changes that have not been applied to the live site have been closed (eg. reviewed and approved), and allow any of the users to refresh the live site upto the latest approved revision. (or even allow a cron to do this automatically)
Implementing this has been a little problematic.
a) The history view on mtrack is very slow, partly due to it pulling out an excessive history (and probably not paging - although i've not looked in detail at this.)
b) The layout of the history view is full of white space. I changed this to look more table like, who/what etc. on the left and commit message on right. To make that work I had to modify a bit of the HTML, which was embedded the PHP code... outch, and change the CSS, which as I mentioned before was a bit tricky due to the CSS merging.
c) I had to hard code the reading of the live site HEAD revision into the code... - not even sure how that could be set up.. - probably as a configuration option.
Adding features - Not everyone can close a ticket.
In our scenario, we did not want the developers to close tickets, only flag them as completed (and ready for review). Again this involved modifying the issue edit page
I'm not quite sure, but it may be a character flaw of mine, is that when I see ineffecient or slightly messy code, I have a urge to clean it up and make it clearer.
For the output of the 'change ticket status' area of the issue edit, most of the code looks like this.
$html .= mtrack_radio('action', 'accept', $_POST['action']);
$html .= " <label for='accept'>Accept ticket</label><br>\n";
obviously a bit of effort has gone into creating a method to output radio buttons, however, since all the code was pretty similar, the temptation was just too much, so I had to tidy that up a bit...
$html .= mtrack_chg_status( 'action', 'accept', "Assign this ticket to ME");
In reality it's a small step forward to tidying up this code, it reduces the size of code in half, and makes it far easier to read.
Interestingly this is one of the problems we have with outsourced developers. They will continue to add and expand code, duplicating the same potential problems rather than reduce and simplify. Which results in code bases of 200+ lines where 10 would have done the same job.
Watching you watching me...
The last change I will talk about today is the watching of issues and commits etc. Most trackers I've seen (and mail on commit hooks) tend to send an email the moment something happens, eg. a commit happened, emails are sent out. someone commented on a issue, an email is sent out.
I suspect Wez was thinking that there may be times when this triggered action, may cause problems (eg. mail server is down or overloaded, so the commit hangs, or the web interface stops responding). Hence he has a 'notification system' where it is logged that somebody, or many people need notifying about something, then a cron job is used to actually run through this notifications and issue the resulting email. (In theory other notification formats could be supported, as the design supports that, but nothing is implemented at present)
Unfortunatly, I think this feature is just beginning to happen, as I had to add a few things to the code to make notification work.
By default nobody get's notified about anything, you have to press 'watch' on either an issue, or a whole repository.
So for our developers, even if we created an issue, and assigned it to them, they would not get an email. Similarly, If for example our issue manager (who does not get commit emails) added a issue, he would not be notified if it was updated unless he pressed watch.
It's quite simple to add this, basically you sign up the submitter and assignee automatically when a issue is saved. The code from the web form can be used to do this, it is quite portable for that, but again it's a copy and paste, rather than a method call (in the end I created a new method for it in the watch class)
Anyway enough for today ;)
